js/lite/src/ietf/publisher.ts (1)

233-242: TODO stubs for v15 request routing — track for follow-up.

Both handleRequestOk and handleRequestError are no-ops with TODO comments about routing by request_id. Since RequestOk/RequestError are generic messages that can correspond to subscribe, publish-namespace, or other requests, implementing the request-id-based dispatch is essential for correct v15 behavior.

Would you like me to open an issue to track implementing the request-id-based routing for RequestOk/RequestError in the publisher?

rs/moq-lite/src/ietf/request.rs (1)

145-156: Consider extending the round-trip test with non-default parameters.

The test uses MessageParameters::default() (empty), so the parameters encoding/decoding path isn't actually exercised beyond writing/reading a zero count. Adding a test case with at least one parameter would increase confidence.

rs/moq-lite/src/ietf/subscribe.rs (1)

94-97: Silent truncation: u64 as u8 for group_order could mask invalid values.

params.group_order() returns Option<u64>. The cast v as u8 silently truncates values > 255. While GroupOrder enum values are small, a malformed parameter could silently produce an incorrect GroupOrder variant or fall through to the unwrap_or(Descending) default.

Consider using u8::try_from(v) with a fallback:

Proposed fix

-				let group_order = match params.group_order() {
-					Some(v) => GroupOrder::try_from(v as u8).unwrap_or(GroupOrder::Descending),
-					None => GroupOrder::Descending,
-				};
+				let group_order = match params.group_order() {
+					Some(v) => u8::try_from(v)
+						.ok()
+						.and_then(|b| GroupOrder::try_from(b).ok())
+						.unwrap_or(GroupOrder::Descending),
+					None => GroupOrder::Descending,
+				};

js/lite/src/ietf/connection.ts (2)

209-216: Sequential routing of RequestOk/RequestError to both handlers risks skipping the second handler on error.

If this.#publisher.handleRequestOk(msg) throws, this.#subscriber.handleRequestOk(msg) is never called (and vice versa for RequestError). Since these generic v15 messages could belong to either side, consider using Promise.allSettled or try-catching individually:

Proposed fix

 			} else if (msg instanceof RequestOk) {
 				// v15: Route RequestOk to both publisher and subscriber
-				await this.#publisher.handleRequestOk(msg);
-				await this.#subscriber.handleRequestOk(msg);
+				await Promise.allSettled([
+					this.#publisher.handleRequestOk(msg),
+					this.#subscriber.handleRequestOk(msg),
+				]);
 			} else if (msg instanceof RequestError) {
 				// v15: Route RequestError to both publisher and subscriber
-				await this.#publisher.handleRequestError(msg);
-				await this.#subscriber.handleRequestError(msg);
+				await Promise.allSettled([
+					this.#publisher.handleRequestError(msg),
+					this.#subscriber.handleRequestError(msg),
+				]);
 			} else {

---

67-70: Constructor accepts optional version — default handled downstream.

The optional version parameter flows to Control.Stream which defaults to DRAFT_14. This is fine, but documenting the default in the JSDoc (line 63) would help callers understand the behavior without tracing through to Control.Stream.

rs/moq-lite/src/ietf/setup.rs (1)

80-84: Variable shadowing: version on line 82 shadows the IetfVersion parameter.

In the Draft14 path, let version = Version::decode(r, version)?; shadows the outer version: IetfVersion with a decoded Version (wire format). Then Parameters::decode(r, version) on line 83 uses this shadowed Version value instead of the IetfVersion enum. This likely works today since Parameters::decode is generic over V, but it's fragile — if Parameters ever needs the IetfVersion enum for version-specific behavior, this would silently break.

Consider renaming to avoid ambiguity:

Proposed fix

 		IetfVersion::Draft14 => {
-			let version = Version::decode(r, version)?;
-			let parameters = Parameters::decode(r, version)?;
-			Ok(Self { version, parameters })
+			let selected_version = Version::decode(r, version)?;
+			let parameters = Parameters::decode(r, version)?;
+			Ok(Self { version: selected_version, parameters })
 		}

rs/moq-lite/src/ietf/publish.rs (1)

388-468: Good test coverage for both Publish and PublishOk across v14/v15.

Consider adding a test for Publish with largest_location: None to cover the no-content-exists path for Draft14 and the missing parameter path for Draft15.

js/lite/src/ietf/parameters.ts (2)

126-250: MessageParameters duplicates Parameters encode/decode logic.

Both classes have identical encode/decode methods and the same vars/bytes map pattern. Consider extracting a shared base class or utility to reduce duplication. Not urgent given the small size, but worth noting for maintainability.

---

199-208: subscriptionFilter getter/setter assumes single-byte varint encoding.

The getter reads data[0] and the setter writes new Uint8Array([v]). This works for current filter type values (0–4) but would break silently for values ≥ 64 where a varint would require multi-byte encoding within the bytes payload. Since the spec-defined filter types are 0–3, this is fine for now.

rs/moq-lite/src/ietf/fetch.rs (1)

340-414: Tests only assert a subset of fields — consider full round-trip assertions.

The v14/v15 round-trip tests for both Fetch and FetchOk skip asserting group_order, fetch_type (for Fetch), and group_order (for FetchOk). A full assert_eq!(decoded, msg) would catch serialization regressions across all fields, since both structs derive PartialEq.

Example

-		assert_eq!(decoded.request_id, RequestId(1));
-		assert_eq!(decoded.subscriber_priority, 128);
+		assert_eq!(decoded, msg);

rs/moq-lite/src/ietf/subscriber.rs (1)

188-206: recv_request_error duplicates recv_subscribe_error logic — consider extracting a shared helper.

Lines 194–206 replicate the subscribe-state cleanup in recv_subscribe_error (lines 175–186). Extracting a small remove_subscribe(request_id) helper would reduce the duplication.

Example shared helper

+	fn remove_subscribe(&mut self, request_id: RequestId) {
+		let mut state = self.state.lock();
+		if let Some(track) = state.subscribes.remove(&request_id) {
+			track.producer.abort(Error::Cancel);
+			if let Some(alias) = track.alias {
+				state.aliases.remove(&alias);
+			}
+		}
+	}
+
 	pub fn recv_subscribe_error(&mut self, msg: ietf::SubscribeError) -> Result<(), Error> {
-		let mut state = self.state.lock();
-		if let Some(track) = state.subscribes.remove(&msg.request_id) {
-			track.producer.abort(Error::Cancel);
-			if let Some(alias) = track.alias {
-				state.aliases.remove(&alias);
-			}
-		}
+		self.remove_subscribe(msg.request_id);
 		Ok(())
 	}

 	pub fn recv_request_error(&mut self, msg: &ietf::RequestError<'_>) -> Result<(), Error> {
-		let mut state = self.state.lock();
-		if let Some(track) = state.subscribes.remove(&msg.request_id) {
-			track.producer.abort(Error::Cancel);
-			if let Some(alias) = track.alias {
-				state.aliases.remove(&alias);
-			}
-		}
+		self.remove_subscribe(msg.request_id);
 		Ok(())
 	}

rs/moq-lite/src/version.rs (1)

66-73: Nit: both Encode arms are identical — could simplify.

Both arms convert to coding::Version and encode. You could collapse this to a single expression since From<Version> for coding::Version is already implemented.

Simplified Encode

 impl<V> coding::Encode<V> for Version {
 	fn encode<W: bytes::BufMut>(&self, w: &mut W, v: V) {
-		match self {
-			Self::Ietf(version) => coding::Version::from(*version).encode(w, v),
-			Self::Lite(version) => coding::Version::from(*version).encode(w, v),
-		}
+		coding::Version::from(*self).encode(w, v)
 	}
 }

rs/moq-lite/src/ietf/parameters.rs (1)

146-160: Encoding emits all varints before all bytes — keys may not be globally sorted.

The encode iterates vars first then bytes, so a varint key 0x22 (GROUP_ORDER) would be emitted before a bytes key 0x03 (AUTHORIZATION_TOKEN), producing globally out-of-order keys. The protocol's count-based decode handles any order, so this isn't a correctness issue, but if strict key ordering is ever expected by a conformance test or peer, it would fail.

If you want globally sorted keys, you'd need to merge-iterate both maps. Low priority since the spec doesn't appear to mandate ordering.

js/lite/src/ietf/control.ts (1)

126-128: Type-unsafe cast to pass optional version to encode — pragmatic but fragile.

The as (w: Writer, v?: IetfVersion) => Promise<void> cast works at runtime because JS ignores extra arguments, but it completely bypasses TypeScript's type checking. If a message's encode signature changes in a breaking way, the compiler won't catch it.

Consider defining a common Encodable interface that all message classes implement, with an optional version parameter, to get compile-time safety.

rs/moq-lite/src/server.rs (1)

46-57: Add clarifying comment explaining why Draft15 is excluded from NEGOTIATED.

The codebase correctly implements ALPN-based version negotiation, but the design choice warrants documentation. NEGOTIATED (defined in version.rs) includes only [Lite(Draft02), Lite(Draft01), Ietf(Draft14)], excluding Ietf(Draft15). This is intentional: Draft15 uses a different encoding format (no versions list in setup messages) and therefore cannot participate in dynamic version negotiation—it requires explicit ALPN negotiation instead.

Consider adding a brief comment to the NEGOTIATED definition or the None case in server.rs line 55 explaining that Draft15 requires ALPN-based negotiation and cannot be selected from the fallback version list. This will help future readers understand the design rationale.

rs/moq-lite/src/setup.rs (2)

25-28: assert_eq! in encode path panics instead of returning an error.

Both Client::encode_inner (line 27) and Server::encode_inner (line 107) use assert_eq! to validate Draft15 invariants. Since Encode doesn't return Result, this is constrained by the trait, but callers constructing a Client with mismatched versions will get an opaque panic. A debug_assert_eq! would be less surprising in release builds, or consider documenting this precondition on the struct.

---

46-48: u64 as usize can silently truncate on 32-bit targets.

Lines 47, 86, 129, and 145 cast decoded u64/u16 values to usize. On 32-bit platforms, a u64 value exceeding u32::MAX would silently truncate, leading to an undersized buffer read and subtle corruption. Consider using usize::try_from(...).map_err(|_| DecodeError::InvalidValue)? for the u64 paths.

Proposed fix (example for line 47)

-			Version::Lite(lite::Version::Draft02 | lite::Version::Draft01) => u64::decode(r, v)? as usize,
+			Version::Lite(lite::Version::Draft02 | lite::Version::Draft01) => {
+				usize::try_from(u64::decode(r, v)?).map_err(|_| DecodeError::InvalidValue)?
+			}

Apply the same pattern at line 145.

js/lite/src/ietf/parameters.ts (1)

128-269: MessageParameters largely duplicates Parameters — consider a shared base or utility.

Both classes have identical vars/bytes maps, identical encode/decode logic, and the same even/odd ID convention. The only difference is the typed accessors on MessageParameters. Extracting the core map + encode/decode into a shared base would reduce duplication.

This is non-blocking for this PR but worth tracking.

rs/moq-lite/src/ietf/fetch.rs (1)

348-422: Round-trip tests don't assert group_order or fetch_type survive the encode/decode cycle.

The v14 and v15 round-trip tests for both Fetch and FetchOk only check request_id, subscriber_priority, end_of_track, and end_location. Consider adding assertions for group_order and fetch_type to confirm these fields are correctly preserved, especially since v15 routes them through MessageParameters.

Example additions for test_fetch_v15_round_trip

 		assert_eq!(decoded.request_id, RequestId(1));
 		assert_eq!(decoded.subscriber_priority, 128);
+		assert_eq!(decoded.group_order, GroupOrder::Descending);
+		assert_eq!(decoded.fetch_type, msg.fetch_type);

rs/moq-native/src/client.rs (1)

331-352: Shadowed alpns on Line 334 may confuse readers.

Inside the if request.url.scheme() == "https" branch, a new alpns is declared that shadows the one from Line 312. The outer alpns (for "https") is ["h3"] (TLS ALPN), while the inner one (Line 334) is all MoQ ALPNs (WebTransport sub-protocol negotiation). The semantics are different but the names are identical.

Consider renaming the inner variable to protocols or sub_protocols to clarify intent:

Suggested rename

 		let session = if request.url.scheme() == "https" {
-			let alpns: Vec<String> = moq_lite::alpns().iter().map(|alpn| alpn.to_string()).collect();
-			let request = request.with_protocols(alpns);
+			let protocols: Vec<String> = moq_lite::alpns().iter().map(|alpn| alpn.to_string()).collect();
+			let request = request.with_protocols(protocols);
 			web_transport_quinn::Session::connect(connection, request).await?

rs/moq-lite/src/ietf/subscribe.rs (1)

56-113: Subscribe v14 decode discards start location from AbsoluteStart and AbsoluteRange filter types.

Lines 69-73 decode optional location fields for AbsoluteStart/AbsoluteRange but discard them with let _start / let _end_group. The struct only stores filter_type without the associated location data. If the intent is to reject absolute filters, consider returning Err(DecodeError::Unsupported) for those cases (similar to how forward == false is rejected on Line 63). If they're intentionally tolerated but not used, this is fine for now but a doc comment would help.

rs/moq-lite/src/ietf/publish.rs (2)

230-237: Repeated group_order decoding pattern — consider extracting a helper.

This exact block appears 6 times across fetch.rs, subscribe.rs, and publish.rs:

let group_order = match params.group_order() {
    Some(v) => u8::try_from(v)
        .ok()
        .and_then(|v| GroupOrder::try_from(v).ok())
        .map(GroupOrder::any_to_descending)
        .unwrap_or(GroupOrder::Descending),
    None => GroupOrder::Descending,
};

A method on MessageParameters (e.g., params.group_order_or_default()) or a free function on GroupOrder would eliminate this duplication.

Possible helper on GroupOrder

impl GroupOrder {
    /// Decode group_order from MessageParameters, defaulting to Descending.
    pub fn from_params(params: &MessageParameters) -> Self {
        match params.group_order() {
            Some(v) => u8::try_from(v)
                .ok()
                .and_then(|v| GroupOrder::try_from(v).ok())
                .map(GroupOrder::any_to_descending)
                .unwrap_or(GroupOrder::Descending),
            None => GroupOrder::Descending,
        }
    }
}

---

396-476: Good test coverage for both Publish and PublishOk across v14 and v15.

Tests verify key fields round-trip correctly. Similar to fetch.rs, you might also want to assert group_order and filter_type survive the encode/decode cycle.

rs/moq-lite/src/client.rs (1)

81-85: Error diagnostic is slightly misleading — both sides of the error are the same list.

client.versions was set from supported.clone().into() on line 70, so the error at line 85 passes what are effectively the same versions for both the "offered" and "supported" arguments. Consider using vec![server.version] as one side to clarify that it was the server's chosen version that didn't match.

rs/moq-native/src/iroh.rs (1)

69-74: Hardcoded ALPN list diverges from the dynamic pattern used by quinn/quiche backends.

The quinn and quiche server implementations iterate over moq_lite::alpns() to populate ALPNs dynamically, whereas here the IETF ALPNs are hardcoded. If a Draft 16 ALPN is added to moq_lite::alpns(), this file will silently miss it.

Consider aligning with the other backends:

♻️ Suggested refactor

 let mut builder = IrohEndpoint::builder().secret_key(secret_key).alpns(vec![
     web_transport_iroh::ALPN_H3.as_bytes().to_vec(),
     moq_lite::lite::ALPN.as_bytes().to_vec(),
-    moq_lite::ietf::ALPN_14.as_bytes().to_vec(),
-    moq_lite::ietf::ALPN_15.as_bytes().to_vec(),
 ]);
+// Dynamically add all moq_lite ALPNs
+let mut alpn_list = vec![
+    web_transport_iroh::ALPN_H3.as_bytes().to_vec(),
+    moq_lite::lite::ALPN.as_bytes().to_vec(),
+];
+for alpn in moq_lite::alpns() {
+    alpn_list.push(alpn.as_bytes().to_vec());
+}
+let mut builder = IrohEndpoint::builder().secret_key(secret_key).alpns(alpn_list);

rs/moq-native/src/quinn.rs (1)

212-220: Potential duplicate ALPNs in server TLS config.

moq_lite::lite::ALPN is added explicitly on line 214, then moq_lite::alpns() is iterated on lines 216–218. If alpns() already includes the lite ALPN, it'll appear twice. Duplicate ALPN entries are generally harmless but could be confusing. This depends on the same question flagged in the iroh review — whether alpns() includes the lite ALPN.